[WC-3429] migrate maps to mobx#2255
Conversation
This comment has been minimized.
This comment has been minimized.
0335d2a to
a079748
Compare
This comment has been minimized.
This comment has been minimized.
- Replace all setTimeout/Promise delays with MobX when() helper - Mock convertAddressToLatLng at module level for deterministic tests - Add waitForLocations() helper using when() for observable changes - Configure MobX with enforceActions: 'never' for testing - Add timeout handling for error cases - Improve test reliability and speed (3.5s vs 13s) - All 42 tests still passing with 100% model layer coverage
Split 598-line test file into 3 self-contained files: - LocationResolver.unit.spec.ts (247 lines) - Basic functionality, empty inputs, API keys - LocationResolver.integration.spec.ts (321 lines) - Mixed markers, caching, errors, integration - LocationResolver.reactivity.spec.ts (226 lines) - MobX reactions and observable behavior Benefits: - Each file self-contained with inline setup (no helpers folder) - Follows Gallery/Datagrid patterns in the repo - Easy to locate specific test types - Can run test files independently - No abstraction layers to learn - Clear test intent without jumping to other files All 45 tests passing with 100% model layer coverage maintained
Add 26 unit tests for convertStaticModeledMarker and convertDynamicModeledMarker: convertStaticModeledMarker (5 tests): - All fields present - Undefined optional fields - Number parsing with comma/period decimal separators - Custom marker image handling convertDynamicModeledMarker (21 tests): - Datasource availability (undefined, Loading, Unavailable, empty) - Coordinates location type (single/multiple markers, missing attributes) - Address location type (with/without address attribute) - Optional fields (title, onClick action, custom marker) - Edge cases (item IDs, NaN handling, empty strings, mixed attributes) Test results: - 26/26 tests passing - 100% code coverage on data.ts - Self-contained tests using @mendix/widget-plugin-test-utils - Uses list(), obj(), listAttribute(), listAction(), dynamic() helpers
Remove test 'should handle NaN from invalid coordinate strings' because: - Dynamic markers use ListAttributeValue<Big>, not string attributes - Mendix runtime ensures type safety - we never receive invalid coordinate types - The scenario being tested doesn't occur in practice Tests: 70 passed (was 71) Coverage: Still 100% on data.ts
Remove 'should handle multiple markers with different attributes' test because: - Already covered by 'should convert multiple markers with coordinates' test - Doesn't add unique value - tests same scenario with different attribute values - Simplifies test suite without losing coverage Tests: 69 passed (was 70) Coverage: Still 100% on data.ts
Improve markers() computed getter: - Remove mutable array and push operations - Store static/dynamic markers in separate variables - Use .flat() instead of .reduce() for flattening - Return immutable spread [...staticMarkers, ...dynamicMarkers] Benefits: - More functional style (no mutations) - Clearer intent with named variables - Modern Array.flat() is more readable than reduce - Shorter and easier to understand Before: const markers = []; markers.push(...static); const flattened = dynamic.reduce((prev, curr) => [...prev, ...curr], []); markers.push(...flattened); After: const staticMarkers = ... const dynamicMarkers = ....flat(); return [...staticMarkers, ...dynamicMarkers]; All tests passing (69/69)
Move marker equality comparison from manual check to reaction equals option: Before: - Manual isIdenticalMarkers() check inside effect - Separate private method for comparison - Early return if markers unchanged After: - Built-in equals option in reaction config - MobX handles comparison before effect runs - Cleaner effect logic without manual check Benefits: - More idiomatic MobX (comparison at framework level) - Simpler effect code (no manual equality check) - Remove isIdenticalMarkers() method (less code) - Same behavior: still compares markers excluding action callbacks The equals function: - Maps prev/next to remove action field - Uses deepEqual with strict mode - Returns true if markers unchanged (prevents effect run) All tests passing (69/69)
Move API key retrieval from inline expression to computed getter: Before: - Inline in reaction effect - const apiKey = this.mainGate.props.geodecodeApiKeyExp?.value ?? this.mainGate.props.geodecodeApiKey; After: - Computed getter property - get apiKey(): string | undefined - Use this.apiKey in reaction Benefits: - Cleaner reaction code (one less variable) - Better MobX reactivity tracking - Reusable if needed elsewhere - Clear documentation via JSDoc - Follows computed pattern for derived values All tests passing (69/69)
Three improvements to LocationResolverService: 1. Version counter instead of reference equality: - Replace requestedMarkers reference check - Use geocodeVersion number (++version pattern) - Clearer intent: 'is this the latest request?' - Minimal memory (number vs full array) 2. Remove runInAction wrapper: - updateLocations is already an action - No need for extra runInAction wrapper - Simpler, cleaner code 3. Geocode function as dependency: - Extract GeocodeFunction type in tokens - Inject via constructor (better testability) - Bind in RootContainer (shared utility) - Remove direct import of convertAddressToLatLng Benefits: - Better dependency injection pattern - Easier to mock in tests - Clearer async handling with version counter - Less memory usage - Follows DI best practices All tests passing (69/69)
Move dependency injection registration from service to container: Before: - Service file had: injected(LocationResolverService, ...) - Container had: injected() call in inject() method - Duplication of DI setup After: - Service file: Clean, no DI registration - Container file: Single source of DI registration - Removed unused imports (injected, CORE_TOKENS) Benefits: - DI setup in one place (container) - Service file is cleaner - Follows separation of concerns - Container owns binding configuration The injected() call now lives only in Maps.container.ts where the binding happens, not scattered in service files. All tests passing (69/69)
Fix 'should update props when they change' test to verify correct behavior: Before: - Checked if config.name updated (it doesn't - config is static) - Test was incomplete and didn't verify actual prop changes After: - Check mainGate.props.name (correct source of reactive props) - Verify props actually update via the gate - Test now validates the real behavior Key insight: - Config is static (bound once at container creation) - MainGate provides reactive access to current props - Services use mainGate.props for up-to-date values The test now correctly verifies that: 1. Container instance remains stable (reusability) 2. MainGate provides updated props (reactivity) All tests passing (69/69)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ests - Create test-utils.ts with createTestContainer helper that uses real MapsContainer - Replace jest.mock() with explicit dependency injection via container - Override geocodeFunction binding before container initialization - Properly trigger setup lifecycle to start MobX reactions - All 22 LocationResolver tests passing with improved type safety and maintainability Benefits: - Tests now use real container setup, catching integration issues - Explicit, type-safe mocking of dependencies - Reusable test utilities for other service tests - 100% coverage of LocationResolverService
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
9570756 to
83f1f81
Compare
AI Code Review
What was reviewed
Skipped (out of scope): CI checks were not available during this review run. Findings
|
Pull request type
No code changes (changes to documentation, CI, metadata, etc.)
Description
Add mobx container for maps, no code changes in pas yet.